Remove equivocating indices from score consideration #5397

zilm13 · 2022-04-27T16:00:33Z

PR Description

Activates ethereum/consensus-specs#2845
Currently in progress, posting draft.

My current TODO:

add something to FFGUpdatesTest
add something to VotesTest
add something to ForkChoiceTest
testing that cli flag works

Fixed Issue(s)

Fixes #5133

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/forkchoice/VoteTracker.java

...m/statetransition/src/main/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoice.java

storage/src/main/java/tech/pegasys/teku/storage/store/Store.java

storage/src/main/java/tech/pegasys/teku/storage/store/StoreTransaction.java

.../beaconchain/src/main/java/tech/pegasys/teku/services/beaconchain/BeaconChainController.java

…uivocating-indices

zilm13 · 2022-04-29T20:30:14Z

TODO:

add ForkChoiceTest with AttesterSlashings in block discarding previous attestations
fix test PR if needed Yield attestation in equivocating indices test ethereum/consensus-specs#2877
don't like serialization/deserialization, looks a bit hacky. We already not versioning it, so we could add version and have something like BeaconBlockBody only with some migration. Feature is not yet activated and we actually may stuck without versioning only when we need another change in votes, so could postpone it till that moment. But maybe you see other good solution

zilm13 · 2022-04-30T18:15:31Z

Test finished, ready for review

ajsutton

In terms of storing a version with vote tracker information, I wouldn't bother. It's quite easy to handle adding additional fields which is all we need here and we're unlikely to need to remove any of the existing fields. If we do in the future we can either add versioning or just discard existing tracked votes - as a one of thing for a migration there won't be any issue with that, we'll just be less able to determine the current head after restart until we start receiving attestations again, so it resolves itself within an epoch and may not make any difference if we need to sync for more than a couple of blocks.

ethereum/core/src/testFixtures/java/tech/pegasys/teku/core/AttesterSlashingGenerator.java

ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/forkchoice/VoteTracker.java

ajsutton · 2022-05-02T22:40:59Z

ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/forkchoice/VoteTracker.java

  }

-  public Bytes32 getNextRoot() {
-    return nextRoot;
+  static VoteTracker markCurrentEquivocating(final VoteTracker voteTracker) {


I don't think this method should exist as when we apply a vote we need to ensure we update all fields, not just equivocating.

We shouldn't update latest_messages vote for equivocating validator as by the spec, and dedicated method ensures that vote fields are not changed
https://github.com/ethereum/consensus-specs/blame/dev/specs/phase0/fork-choice.md#L365
Or perhaps, I don't understand what do you mean

...m/statetransition/src/main/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoice.java

storage/src/main/java/tech/pegasys/teku/storage/protoarray/ForkChoiceStrategy.java

ajsutton · 2022-05-02T22:58:55Z

storage/src/main/java/tech/pegasys/teku/storage/protoarray/ProtoArrayScoreCalculator.java

+      if (vote.isNextEquivocating()) {
+        newVote = VoteTracker.markCurrentEquivocating(vote);
+      } else {
+        newVote = VoteTracker.create(vote.getNextRoot(), vote.getNextRoot(), vote.getNextEpoch());
+      }


I'd just simplify this to:

Suggested change

if (vote.isNextEquivocating()) {

newVote = VoteTracker.markCurrentEquivocating(vote);

} else {

newVote = VoteTracker.create(vote.getNextRoot(), vote.getNextRoot(), vote.getNextEpoch());

}

newVote = new VoteTracker(vote.getNextRoot(), vote.getNextRoot(), vote.getNextEpoch(), vote.isNextEquivocating(), vote.isNextEquivocating());

it probably doesn't matter if we don't actually update the current roots once the validator is equivocating but since it's simple, it makes sense to get our records straight and show that we have considered that next vote now.

Returning to this one. I've applied your suggestion but according to the spec, we shouldn't update votes for equivocated validators. This shouldn't hurt anything from the first look, but it could be our bug in the future

I've reverted this one too, but if you think it's safe to update it, I will put it back

.../main/java/tech/pegasys/teku/storage/server/kvstore/serialization/VoteTrackerSerializer.java

Co-authored-by: Adrian Sutton <adrian@symphonious.net>

…ng indices are on or off Co-authored-by: Adrian Sutton <adrian@symphonious.net>

zilm13 · 2022-05-09T10:35:02Z

@ajsutton
I have two questions before continue to update this one:

You've suggested to update votes for equivocated validators, but I think it's against the spec https://github.com/ethereum/consensus-specs/blame/dev/specs/phase0/fork-choice.md#L365 and it could be a source for the future bugs, are you pretty sure we are on safe path here?
Different serialization depending on Teku flag would require to pass this flag with SpecConfig or something like that. DB is currently pretty independent of everything but the spec which is pretty rational as spec could touch the schema. Do we really want to pass TekuConfiguration there and make DB dependent on that? My current "versions" approach keeps this independence. Maybe it could be tweaked in different manner but making DB module dependent on TekuConfiguration for this case looks a bit unpleasant from my side

ajsutton · 2022-05-09T10:43:13Z

I'll have to look at the spec in more detail but our implementation of fork choice is very different from the spec version. It's probably not a big deal which way it goes to be honest.

I don't think the database needs all of TekuConfiguration passed through but it already has a number of config items that affect how it behaves. The current version means everything that deals with this is handling multiple versions but we want to isolate the feature toggling to just the key entry points (eg the database)

zilm13 · 2022-05-09T10:46:06Z

Generally I agree that from some angle equivocating indices feature toggle looks like a part of spec (it's a part of spec btw :)

…rializer

…lexibility

ajsutton · 2022-05-09T22:18:25Z

So I think you're misunderstanding what the spec is saying vs what our code is doing because while our code gives the same results as the spec the implementation is very very different. The high level overview of it is that the spec applies attestations in a single step - by adding them to store.latest_messages where as our protoarray implementation applies things in two steps. First we update VoteTracker to set the next values to the vote we received. Then later we update weightings from a whole batch of changed attestations at once - when we do that we set the current values to the next ones so that we can tell what's changed and not changed on the next run.

So for us, not applying an attestation from an equivocating validator means not updating the next values when we receive a new attestation from them. We have that check in ForkChoiceStrategy.processAttestation. And for removing the existing weighting we have the check in ProtoArrayScoreCalculator to set the new balance to 0 if nextEquivocating is true.

The idea of next and current doesn't exist in the spec so it's just for our record keeping and it's more accurate to say that we have applied the latest changes for that validator (so current and next have the same values).

zilm13 · 2022-05-10T06:53:28Z

Thank you for explanation. I finally got it. The same way we set intention to change vote's target epoch, and incarnate it later in ProtoArrayCalculator we do with equivocation. And it's safe as we haven't changed equivocating vote root in ForkChoice. Ready for review.

ajsutton

Looks good, just need to move the config out of SpecConfig to StorageConfiguration (it will likely mean passing it through as a boolean in a few layers but that will only be temporary until we remove the feature toggle again).

ajsutton · 2022-05-10T23:46:50Z

ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfig.java

@@ -135,6 +135,8 @@ static SpecConfigBuilder builder() {

  int getProposerScoreBoost();

+  boolean isEquivocatingIndicesEnabled();


hmm, I don't think this belongs in SpecConfig as it's not coming from the config the spec provides. For the storage side it should just wind up in StorageConfiguration.

zilm13 · 2022-05-12T21:10:07Z

@ajsutton did you mean something like this?

ajsutton

Yep spot on. Thanks.

zilm13 added 3 commits April 27, 2022 18:48

Add equivocating validator indices

3c9ddd3

Merge branch 'master' into equivocating-indices

f9bc84c

Fix spotless

e3b848f

ajsutton reviewed Apr 28, 2022

View reviewed changes

zilm13 added 12 commits April 28, 2022 11:31

Remove unused logic in AttesterSlashing handling

10405cd

Copy equivocatingIndices set as it could change during usage

737ac14

Add AttesterSlashings to equivocating indices from imported blocks

968edb3

Disable equivocating validator scores using only votes

47fc31d

Merge branch 'master' into equivocating-indices

913f7ae

Add votes serialize/deserialize test for V2

2a9b23b

Fix style

a4c3c31

Add equivocation test path in VotesTest

1e585c7

Apply equivocation validators infor from block before votes

6048165

Apply equivocation validators info from block before votes

76a2cb1

Merge branch 'equivocating-indices' of github.com:zilm13/teku into eq…

703feef

…uivocating-indices

Rename equivocating vote flags

bc80f38

Add ForkChoiceTest with equivocating AttesterSlashings in block

5af2536

zilm13 marked this pull request as ready for review April 30, 2022 18:15

ajsutton reviewed May 2, 2022

View reviewed changes

zilm13 and others added 4 commits May 3, 2022 17:47

Access AttestationUtil in more convenient style

1949178

Access AttesterSlashings from block with better style

e236bb0

Co-authored-by: Adrian Sutton <adrian@symphonious.net>

Create VoteTracker with better style

d376a96

Co-authored-by: Adrian Sutton <adrian@symphonious.net>

Create VoteTracker with one constructor, no matter whether equivocati…

b45c2b0

…ng indices are on or off Co-authored-by: Adrian Sutton <adrian@symphonious.net>

zilm13 added 2 commits May 9, 2022 18:07

Refactor equivocating VoteTracker, removed versions, use different se…

92f7be9

…rializer

Merge branch 'master' into equivocating-indices

803e736

Initialize ForkChoice with equivocatingIndicesEnabled flag for more f…

48ba057

…lexibility

zilm13 requested a review from ajsutton May 9, 2022 16:50

zilm13 added 2 commits May 10, 2022 08:59

Update validator record in ProtoArray for equivocating validator too

bd12159

Merge branch 'master' into equivocating-indices

098d0e1

ajsutton reviewed May 10, 2022

View reviewed changes

zilm13 added 2 commits May 12, 2022 22:20

Merge branch 'master' into equivocating-indices

de6f976

Move whether to store equivocation in votes to StorageConfiguration

c286b00

ajsutton approved these changes May 13, 2022

View reviewed changes

Merge branch 'master' into equivocating-indices

2a7d92f

ajsutton merged commit 512c941 into Consensys:master May 13, 2022

zilm13 deleted the equivocating-indices branch May 13, 2022 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove equivocating indices from score consideration #5397

Remove equivocating indices from score consideration #5397

zilm13 commented Apr 27, 2022 •

edited

Loading

zilm13 commented Apr 29, 2022

zilm13 commented Apr 30, 2022

ajsutton left a comment

ajsutton May 2, 2022

zilm13 May 9, 2022

ajsutton May 2, 2022

zilm13 May 9, 2022 •

edited

Loading

zilm13 May 9, 2022

zilm13 commented May 9, 2022

ajsutton commented May 9, 2022

zilm13 commented May 9, 2022 •

edited

Loading

ajsutton commented May 9, 2022

zilm13 commented May 10, 2022

ajsutton left a comment

ajsutton May 10, 2022

zilm13 commented May 12, 2022

ajsutton left a comment

		@@ -135,6 +135,8 @@ static SpecConfigBuilder builder() {

		int getProposerScoreBoost();

		boolean isEquivocatingIndicesEnabled();

Remove equivocating indices from score consideration #5397

Remove equivocating indices from score consideration #5397

Conversation

zilm13 commented Apr 27, 2022 • edited Loading

PR Description

Fixed Issue(s)

Documentation

Changelog

zilm13 commented Apr 29, 2022

zilm13 commented Apr 30, 2022

ajsutton left a comment

Choose a reason for hiding this comment

ajsutton May 2, 2022

Choose a reason for hiding this comment

zilm13 May 9, 2022

Choose a reason for hiding this comment

ajsutton May 2, 2022

Choose a reason for hiding this comment

zilm13 May 9, 2022 • edited Loading

Choose a reason for hiding this comment

zilm13 May 9, 2022

Choose a reason for hiding this comment

zilm13 commented May 9, 2022

ajsutton commented May 9, 2022

zilm13 commented May 9, 2022 • edited Loading

ajsutton commented May 9, 2022

zilm13 commented May 10, 2022

ajsutton left a comment

Choose a reason for hiding this comment

ajsutton May 10, 2022

Choose a reason for hiding this comment

zilm13 commented May 12, 2022

ajsutton left a comment

Choose a reason for hiding this comment

zilm13 commented Apr 27, 2022 •

edited

Loading

zilm13 May 9, 2022 •

edited

Loading

zilm13 commented May 9, 2022 •

edited

Loading